Skip to content

feat(globals): runtime disable + scripts:globals hook#808

Merged
harlan-zw merged 3 commits into
mainfrom
feat/scripts-globals-runtime-hook
Jun 2, 2026
Merged

feat(globals): runtime disable + scripts:globals hook#808
harlan-zw merged 3 commits into
mainfrom
feat/scripts-globals-runtime-hook

Conversation

@harlan-zw
Copy link
Copy Markdown
Collaborator

🔗 Linked issue

Related to #759

❓ Type of change

  • 📖 Documentation
  • 🐞 Bug fix
  • 👌 Enhancement
  • ✨ New feature
  • 🧹 Chore
  • ⚠️ Breaking change

📚 Description

Follow-up to #780. scripts.globals entries are resolved at build time, so a single build can't drop or recompute one per instance. The env override added in #780 only mutates values of existing keys, so unused integrations still register on every deployment. This adds two runtime escapes, both keyed off the existing scriptsGlobals override:

  • Disable per instance. Registration routes through a guard that skips an entry whose merged input has enabled: false or an empty/null src. A tenant that doesn't use Awin drops it with NUXT_PUBLIC_SCRIPTS_GLOBALS_AWIN_SRC=, no rebuild. The key resolves to undefined on $scripts.
  • scripts:globals runtime hook. The generated scripts:init plugin builds a mutable map of the resolved inputs and fires the hook before registering, so userland can rewrite src, delete keys, or add entries from runtime config. Register the listener in an enforce: 'pre' plugin so it runs first.

I used a hook rather than a globals(runtimeConfig) factory because globals are resolved at build to generate the $scripts types and feed asset bundling; a factory at server startup happens after that and loses both. The hook keeps statically declared entries typed and bundled while letting you compute the rest at startup.

Covered by unit tests (guard logic, hook ordering, generated map shape) and the issue-759 e2e (env override, disable-via-empty-src, hook rewrites src, all against a built fixture).

harlan-zw added 2 commits June 2, 2026 12:59
Globals are resolved at build time, so a single build couldn't drop or
recompute an entry per instance. Two runtime escapes, both keyed off the
existing scriptsGlobals override:

- Registration now routes through a guard that skips an entry whose merged
  input has enabled: false or an empty/null src, so an unused integration
  can be dropped per deployment via NUXT_PUBLIC_SCRIPTS_GLOBALS_<KEY>_SRC=.
- The generated plugin builds a mutable map of resolved inputs and fires a
  scripts:globals app hook before registering, letting userland rewrite src,
  delete keys, or add entries from runtime config. Static globals keep their
  $scripts types and asset bundling, which a globals factory would lose.

Related to #759
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Jun 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
scripts-playground Ready Ready Preview, Comment Jun 2, 2026 3:19am

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Jun 2, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@nuxt/scripts@808

commit: 28c5c12

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds a new Nuxt App hook, scripts:globals, and updates the plugin template to collect globals into a mutable __globals map, invoke the hook before registration, and use a guarded __registerGlobal helper that skips disabled or empty-src entries. It makes generated setup async when globals exist, documents the hook and per-instance env overrides, and updates unit, e2e, and fixture tests to validate rewrite/deletion/disable semantics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title concisely and accurately summarizes the two main features: runtime disable for globals and the new scripts:globals hook, which are the primary changes across the affected files.
Description check ✅ Passed The description clearly explains the motivation, implementation details, and coverage of the changes, directly relating to the modifications across docs, templates, and tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/scripts-globals-runtime-hook

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
test/fixtures/issue-759/plugins/globals.ts (1)

6-6: ⚡ Quick win

Let the callback infer the real scripts:globals hook type.

The explicit Record<string, Record<string, any>> annotation masks regressions in the new hook typing this fixture is supposed to exercise. Dropping it keeps the fixture pinned to the generated Nuxt hook contract.

♻️ Proposed fix
-    nuxtApp.hooks.hook('scripts:globals', (globals: Record<string, Record<string, any>>) => {
+    nuxtApp.hooks.hook('scripts:globals', (globals) => {
       globals.scrads.src = 'https://scrads.example/from-hook.js'
     })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/fixtures/issue-759/plugins/globals.ts` at line 6, Remove the explicit
type annotation on the callback parameter so the hook callback can infer the
real "scripts:globals" hook type; locate the nuxtApp.hooks.hook call
(nuxtApp.hooks.hook('scripts:globals', (globals: Record<string, Record<string,
any>>) => { ... })) and change it to use an untyped parameter (e.g., (globals)
=> { ... }) so the fixture exercises the generated Nuxt hook contract rather
than masking it.
test/e2e/issue-759-globals-env-override.test.ts (1)

33-39: ⚡ Quick win

Assert the registered $scripts.scrads value too.

This only proves the rewritten href made it into the HTML head. It can still pass if the preload/link path updates while $scripts.scrads keeps the baked src. app.vue already exposes #scrads-src, so add an assertion for that rendered value as well.

♻️ Proposed fix
   it('the scripts:globals hook rewrites a global src at runtime', async () => {
     const html = await $fetch<string>('/')
     // The hook-mutated src is what actually gets registered/preloaded...
     expect(html).toContain('href="https://scrads.example/from-hook.js"')
+    expect(html).toContain('<div id="scrads-src">https://scrads.example/from-hook.js</div>')
     // ...while the un-mutated build-time src is never used for registration.
     expect(html).not.toContain('href="https://scrads.example/baked.js"')
+    expect(html).not.toContain('<div id="scrads-src">https://scrads.example/baked.js</div>')
   })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/e2e/issue-759-globals-env-override.test.ts` around lines 33 - 39, Add an
assertion to the test that verifies the runtime-registered $scripts.scrads value
was updated by the scripts:globals hook by reading the DOM element rendered by
app.vue with id "`#scrads-src`" (exposed in the app) and asserting its
text/content equals "https://scrads.example/from-hook.js"; keep existing HTML
head assertions but add this extra check so $scripts.scrads is validated as
rewritten at runtime.
test/unit/templates.test.ts (1)

342-361: ⚡ Quick win

Add coverage for a deleted/undefined entry.

The guard unit test exercises enabled: false, empty/null/undefined src, but not the case where the entry itself is undefined (i.e. a hook delete globals.<key>). That path currently throws on destructuring (see the critical issue in templates.ts). After the guard fix, add expect(__registerGlobal(undefined, {})).toBeUndefined() so the regression is locked in.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/unit/templates.test.ts` around lines 342 - 361, Add a test asserting the
deleted/undefined entry case by adding expect(__registerGlobal(undefined,
{})).toBeUndefined() to the "__registerGlobal skips disabled entries and
registers the rest" test, and ensure the implementation of __registerGlobal in
templates.ts defensively handles an undefined input (e.g., early return before
destructuring) so it no longer throws when called with undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/script/src/templates.ts`:
- Around line 242-255: The generated __registerGlobal currently destructures its
argument unguarded and throws when a hook deletes an entry; update
__registerGlobal (the function used to wrap useScript calls) to guard against a
missing/falsy input before destructuring (e.g. return undefined immediately if
!input or use a default param) so deleted entries from __globals don't crash
plugin setup; keep the existing checks for enabled/empty/null src and ensure
call sites in the setupBody that build __globals and globalInits remain
compatible. Also update the inline snapshot and the unit tests in
test/unit/templates.test.ts to reflect the new behavior and add a test case for
a deleted/global entry.
- Around line 248-257: The current implementation builds registrations only from
the static config.globals (globalMapEntries/globalInits) and never handles keys
added by the runtime hook after await nuxtApp.hooks.callHook('scripts:globals',
__globals); update templates.ts so that immediately after calling the hook you
re-scan Object.keys(__globals) and for any key not already handled push a
registration/init that calls __registerGlobal(__globals["<key>"], ...) (add to
globalInits or inits as appropriate) and ensure the final provide.scripts list
is derived from Object.keys(__globals) combined with resolvedRegistryKeys
instead of only Object.keys(config.globals), so runtime-added keys are
registered and exposed; retain existing behavior for statically declared keys
and document/type-note that bundlers/types remain static.
- Around line 29-31: The generated type for the 'scripts:globals' hook is
currently a closed object with required properties, causing TS2790 when
deleting/adding keys; update the template in packages/script/src/templates.ts
that builds the 'scripts:globals' signature so each mapped key is optional (e.g.
use `?:`) and include an index signature like `[key: string]: Record<string,
any>` so unknown additions and deletions type-check; adjust the globalsKeys.map
generation to emit optional properties and append the index signature to the
produced object type.

---

Nitpick comments:
In `@test/e2e/issue-759-globals-env-override.test.ts`:
- Around line 33-39: Add an assertion to the test that verifies the
runtime-registered $scripts.scrads value was updated by the scripts:globals hook
by reading the DOM element rendered by app.vue with id "`#scrads-src`" (exposed in
the app) and asserting its text/content equals
"https://scrads.example/from-hook.js"; keep existing HTML head assertions but
add this extra check so $scripts.scrads is validated as rewritten at runtime.

In `@test/fixtures/issue-759/plugins/globals.ts`:
- Line 6: Remove the explicit type annotation on the callback parameter so the
hook callback can infer the real "scripts:globals" hook type; locate the
nuxtApp.hooks.hook call (nuxtApp.hooks.hook('scripts:globals', (globals:
Record<string, Record<string, any>>) => { ... })) and change it to use an
untyped parameter (e.g., (globals) => { ... }) so the fixture exercises the
generated Nuxt hook contract rather than masking it.

In `@test/unit/templates.test.ts`:
- Around line 342-361: Add a test asserting the deleted/undefined entry case by
adding expect(__registerGlobal(undefined, {})).toBeUndefined() to the
"__registerGlobal skips disabled entries and registers the rest" test, and
ensure the implementation of __registerGlobal in templates.ts defensively
handles an undefined input (e.g., early return before destructuring) so it no
longer throws when called with undefined.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2b689a21-1bad-4c68-810e-dccedb91351a

📥 Commits

Reviewing files that changed from the base of the PR and between ac0ad5c and 26a4c94.

📒 Files selected for processing (8)
  • docs/content/docs/1.guides/4.global.md
  • docs/content/docs/3.api/6.nuxt-app-hooks.md
  • packages/script/src/templates.ts
  • test/e2e/issue-759-globals-env-override.test.ts
  • test/fixtures/issue-759/app.vue
  • test/fixtures/issue-759/nuxt.config.ts
  • test/fixtures/issue-759/plugins/globals.ts
  • test/unit/templates.test.ts

Comment thread packages/script/src/templates.ts Outdated
Comment thread packages/script/src/templates.ts
Comment thread packages/script/src/templates.ts
Addresses CodeRabbit review on #808:

- __registerGlobal guards against a missing input, so a listener that does
  `delete globals[key]` no longer throws 'Cannot destructure ... undefined'
  and abort plugin setup. Covered by a unit case and an e2e delete case.
- Hook type is an index signature (Record<string, Record<string, any>>) so
  `delete globals[key]` and `globals[key].src = ...` both typecheck (no TS2790
  / possibly-undefined).
- Docs no longer claim the hook can add brand-new globals; registration only
  iterates declared keys, so net-new scripts go through useScript() in a plugin.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/unit/templates.test.ts (1)

319-322: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix the runtime-hook contract in this test comment.

The generated plugin only registers statically declared keys, so brand-new entries added to __globals will not be registered. Saying the hook can “rewrite/delete/add” entries here conflicts with the documented behavior and is likely to mislead future readers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/unit/templates.test.ts` around lines 319 - 322, Update the test comment
for "globals are passed through the scripts:globals runtime hook before
registration" to reflect the actual runtime-hook contract: the generated plugin
only registers statically declared keys so the hook may rewrite or delete
existing entries on __globals but cannot add brand-new keys that will be
registered; reference the scripts:globals runtime hook and __globals in the
comment so readers know additions won't be picked up by the static registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@test/unit/templates.test.ts`:
- Around line 319-322: Update the test comment for "globals are passed through
the scripts:globals runtime hook before registration" to reflect the actual
runtime-hook contract: the generated plugin only registers statically declared
keys so the hook may rewrite or delete existing entries on __globals but cannot
add brand-new keys that will be registered; reference the scripts:globals
runtime hook and __globals in the comment so readers know additions won't be
picked up by the static registration.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f2c7a00c-54fd-4307-a157-4c3dde3c8896

📥 Commits

Reviewing files that changed from the base of the PR and between 26a4c94 and 28c5c12.

📒 Files selected for processing (8)
  • docs/content/docs/1.guides/4.global.md
  • docs/content/docs/3.api/6.nuxt-app-hooks.md
  • packages/script/src/templates.ts
  • test/e2e/issue-759-globals-env-override.test.ts
  • test/fixtures/issue-759/app.vue
  • test/fixtures/issue-759/nuxt.config.ts
  • test/fixtures/issue-759/plugins/globals.ts
  • test/unit/templates.test.ts
✅ Files skipped from review due to trivial changes (2)
  • docs/content/docs/1.guides/4.global.md
  • test/fixtures/issue-759/app.vue
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/fixtures/issue-759/plugins/globals.ts
  • docs/content/docs/3.api/6.nuxt-app-hooks.md
  • packages/script/src/templates.ts

@harlan-zw harlan-zw merged commit 62b0417 into main Jun 2, 2026
19 checks passed
@harlan-zw harlan-zw deleted the feat/scripts-globals-runtime-hook branch June 2, 2026 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant